-
Notifications
You must be signed in to change notification settings - Fork 8k
arch/arm/cortex_m: support for bridge to the next image S2RAM routines #96290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
arch/arm/cortex_m: support for bridge to the next image S2RAM routines #96290
Conversation
30183a0
to
8ac3f47
Compare
arch/arm/core/cortex_m/reset.S
Outdated
#endif /* CONFIG_INIT_ARCH_HW_AT_BOOT */ | ||
|
||
#if defined(CONFIG_PM_S2RAM) | ||
#if defined(CONFIG_PM_S2RAM) || defined(CONFIG_PM_S2RAM_RESUME_INTERMEDIARY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC correctly you are trying to define CONFIG_PM_S2RAM in zephyr while the other intermediary in the bootloader (which is a zephyr application)?
if thats the case why not just 2 different definitions of pm_s2ram_mark_check_and_clear? 1 for zephyr and other for the bootoader and arch_pm_s2ram_resume calls whichever is applicable here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - that also possible. But the I have to enabled CONFIG_PM=y and CONFIG_PM_S2RAM=y, the overwrite SoC original pm_s2ram_mark_check_and_clear implementation. We already did something like you are suggesting on our forks (https://github.com/nrfconnect/sdk-zephyr/pull/3252/files + https://github.com/nrfconnect/sdk-mcuboot/pull/489/files). So that could fly for sure.
I want to avoid CONFIG_PM=y and CONFIG_PM_S2RAM=y which causes unnecessary code compiled in the MCUboot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change have to be in mcuboot? if mcuboot is just a zephyr application then you could define pm_s2ram_mark_check_and_clear
in soc.c under flags that enable mcuboot.
IMHO, this change is duplicating what pm_s2ram_mark_check_and_clear
was designed for and so doesn't look right.
subsys/pm/Kconfig
Outdated
timer is due to expire. | ||
|
||
config PM_S2RAM_RESUME_INTERMEDIARY | ||
bool "Resume intermeniary fo Suspend-to-RAM (S2RAM)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
subsys/pm/Kconfig
Outdated
power management subsystem of the number of ticks until the next kernel | ||
timer is due to expire. | ||
|
||
config PM_S2RAM_RESUME_INTERMEDIARY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option is specific to bootloaders, and specific to CORTEX_M, it should not be in the common PM kconfig as it is here. The only knowledge needed by any arch is is whether CONFIG_PM_S2RAM is enabled in the application, if that requires enabling an option like this in the case of CORTEX_M with a zephyr based bootloader, that option should depend on CONFIG_PM_S2RAM
, it should not be in PM.
The way to achieve this portably is to use sysbuild. We don't want CONFIG_PM enabled in the bootloader, it means something very different from what the option in this PR does for the bootloader, and we don't need to, we just need to check if the application was built with CONFIG_PM_S2RAM, like we check if the app was built with CONFIG_MCUBOOT_MODE_SINGLE_APP
. See https://github.com/zephyrproject-rtos/zephyr/blob/main/share/sysbuild/images/bootloader/Kconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where you see check for that CONFIG_MCUBOOT_MODE_SINGLE_APP is set for the application? Isn't rather that mode is selected at sysbuild level and the transferred to subimages (via relevant Kconfigs)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the way to do this without sysbuild would be to select an arch specific kconfig manually when building the mcuboot image, that same config could be selected automatically by sysbuild
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config ARCH_PM_S2RAM_RESUME
bool "Include PM S2RAM resume jump"
for case when the zephyr-rtos application is the bootloader and booted target application had suspended the device, the resume operation would be initiated by the bootloader which could redirect execution to the application S2RAM routines directly. Thanks to that target application would resume using compiled in S2RAM routines. Such scheme allows the zephyr-rtos based bootloader to not mock the application while does S2RAM resume operation. Therefore no need for keeping compatibility with S2RAM resume mechanism in application. No need to enable PM nor PM_S2RAM anymore in the bootloader. Signed-off-by: Andrzej Puzdrowski <[email protected]>
8ac3f47
to
cdcc1f5
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at #95914 (comment)
* @brief Check suspend-to-RAM marking and does mediation. | ||
* | ||
* Function does resume mediation if determines resuming after suspend-to-RAM | ||
* or return so standard boot will be executed. | ||
* | ||
* Implementation is up to given application - usually a bootloader. The function is expected | ||
* to do mediation needed for resuming the application from S2AM state, which usually means no | ||
* return to caller. Usage of this API implementation shall be enabled using | ||
* CONFIG_ARCH_PM_S2RAM_RESUME. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of pm_s2ram_mark_check_and_mediate
? What does resume mediation
mean? I don't really understand what an implementation is supposed to do with just this description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is expected to do anything what is needed to boot the device from S2RAM sleep. Usually it should jump to the reset vector of the application. This reset vector is known for the bootloader - it is up to bootloader to make this operation reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a trampoline to redirect reset.S
's bl arch_pm_s2ram_resume
to pm_s2ram_mark_check_and_mediate
? If so, we don't need assembly for that (and indeed @wearyzen's proposal of a platform hook instead is better)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not a part of PM_S2RAM feature compiled in the zephyr-rtos instance. Bootloader should use config CONFIG_ARCH_PM_S2RAM_RESUME and Not CONFIG_PM_S2RAM while the application just the opposite way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key point of my comment was that we don't need a new ASM file for a call trampoline - either inline it in reset.S
(i.e., soc_resume_hook
which is what we're going towards) or implement it in a C file.
(the former solution is probably better)
That PR fixes stacks interference issue. AFIKT S2RAM resume via bootloader is affected by that issue regardless of the existence of this PR solution. @wearyzen soc_resume_hook is not in the right place for objective of that patch. |
So what I wanted to highlight in that comment was that, this PR should introduce a new |
@wearyzen Something like this will make us able to do that. #if defined(CONFIG_SOC_RESUME_HOOK)
bl soc_resume_hook
#endif
#if defined(CONFIG_PM_S2RAM)
/*
* Temporarily set MSP to interrupt stack so that arch_pm_s2ram_resume can
* use stack for calling pm_s2ram_mark_check_and_clear.
@@ -113,6 +124,7 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start)
* a short while, there is no change in behavior in either of the paths.
*/
ldr r0, =z_interrupt_stacks + CONFIG_ISR_STACK_SIZE + MPU_GUARD_ALIGN_AND_SIZE
#endif |
This is how I think the solution would look like: reset.S would call soc_resume_hook()
in soc.c or any soc specific file:
and the pm_s2ram_mark_check_and_mediate goes in the same soc specific file as well. soc_resume_hook should be added similar to how soc_reset_hook was added (not arch specific) and the only change that goes in arch is calling the |
saw this after I hit my reply but we are thinking on same lines now 😄 |
I'm closing this PR in faworu to more generic approach to problemy I want to solve. |
@wearyzen ^^ |
For case when the zephyr-rtos application is the bootloader and booted target application had suspended the device, the resume operation would be initiated by the bootloader which could redirect execution to the application S2RAM routines directly.
Thanks to that target application would resume using compiled in S2RAM routines. Such scheme allows the zephyr-rtos based bootloader to not mock the application while does S2RAM resume operation. Therefore no need for keeping compatibility with S2RAM resume mechanism in application. No need to enable PM nor PM_S2RAM anymore in the bootloader.